GH-41481: [CI] Update how extra environment variables are specified for the integration test docker job #42009

paleolimbot · 2024-06-06T13:04:52Z

Rationale for this change

Currently, nanoarrow and Rust are not being included in the integration tests. The command issued by archery includes multiple environment variable definitions and the rightmost ones disable the extra environment variables.

https://github.com/apache/arrow/actions/runs/9397807525/job/25881776553#step:9:353

DEBUG:archery:Executing `['docker', 'run', '--rm', '-e', 'ARCHERY_DEFAULT_BRANCH=main', '-e', 'ARCHERY_INTEGRATION_WITH_NANOARROW=1', '-e', 'ARCHERY_INTEGRATION_WITH_RUST=1', '-e', 'ARCHERY_INTEGRATION_WITH_NANOARROW=0', '-e', 'ARCHERY_INTEGRATION_WITH_RUST=0', '-e', 'ARROW_CPP_EXE_PATH=/build/cpp/debug', '-e', 'ARROW_NANOARROW_PATH=/build/nanoarrow', '-e', 'ARROW_RUST_EXE_PATH=/build/rust/debug', '-e', 'CCACHE_COMPILERCHECK=content', '-e', 'CCACHE_COMPRESS=1', '-e', 'CCACHE_COMPRESSLEVEL=6', '-e', 'CCACHE_DIR=/ccache', '-e', 'CCACHE_MAXSIZE=1G', '-e', 'GITHUB_ACTIONS=true', '-v', '/home/runner/work/arrow/arrow:/arrow', '-v', '/home/runner/work/arrow/arrow/.docker/conda-ccache:/ccache', 'apache/arrow-dev:amd64-conda-integration', '/arrow/ci/scripts/integration_arrow_build.sh /arrow /build && /arrow/ci/scripts/integration_arrow.sh /arrow /build']`
# ...
+ /arrow/ci/scripts/rust_build.sh /arrow /build
=====================================================================
Not building the Rust implementation.
=====================================================================
+ /arrow/ci/scripts/nanoarrow_build.sh /arrow /build
=====================================================================
Not building nanoarrow
=====================================================================

What changes are included in this PR?

This PR updates how environment variables are specified such that the intended value is passed to the docker build.

Are these changes tested?

Yes

Are there any user-facing changes?

No

GitHub Issue: [Archery][CI][Integration] Integration workflow is no longer running Rust integration tests #41481

github-actions · 2024-06-06T13:05:21Z

⚠️ GitHub issue #41481 has been automatically assigned in GitHub to PR creator.

kou

BTW, can we prefer -e specified by command line to default values in docker-compose.yml instead of this approach? Something like:

diff --git a/dev/archery/archery/docker/core.py b/dev/archery/archery/docker/core.py
index cb83106002..16e164562d 100644
--- a/dev/archery/archery/docker/core.py
+++ b/dev/archery/archery/docker/core.py
@@ -340,18 +340,8 @@ class DockerCompose(Command):
         service = self.config.get(service_name)
 
         args = []
-        if user is not None:
-            args.extend(['-u', user])
-
-        if env is not None:
-            for k, v in env.items():
-                args.extend(['-e', '{}={}'.format(k, v)])
-
-        if volumes is not None:
-            for volume in volumes:
-                args.extend(['--volume', volume])
-
-        if self.config.using_docker or service['need_gpu'] or resource_limit:
+        use_docker = self.config.using_docker or service['need_gpu'] or resource_limit
+        if use_docker:
             # use gpus, requires docker>=19.03
             if service['need_gpu']:
                 args.extend(['--gpus', 'all'])
@@ -396,6 +386,18 @@ class DockerCompose(Command):
             # name which we refer as image in general
             args.append(service['image'])
 
+        if user is not None:
+            args.extend(['-u', user])
+
+        if env is not None:
+            for k, v in env.items():
+                args.extend(['-e', '{}={}'.format(k, v)])
+
+        if volumes is not None:
+            for volume in volumes:
+                args.extend(['--volume', volume])
+
+        if use_docker:
             # add command from compose if it wasn't overridden
             if command is not None:
                 args.append(command)

ci/scripts/nanoarrow_build.sh

ci/scripts/rust_build.sh

paleolimbot · 2024-06-11T13:37:28Z

@kou Thank you for taking a look and apologies for taking a few days to circle back!

BTW, can we prefer -e specified by command line to default values in docker-compose.yml instead of this approach?

That is certainly what I would have expected; however, I am not sure that I feel confident modifying this part of the code. In any case, I think the change here that enables these specific jobs to run is a good one (everything pertaining to whether the tests do or do not run is controlled in the workflow file).

pitrou · 2024-06-13T13:00:23Z

ci/scripts/nanoarrow_build.sh

-  echo "The nanoarrow source is missing. Please clone the arrow-nanoarrow repository"
-  echo "to arrow/nanoarrow before running the integration tests:"
-  echo "  git clone https://github.com/apache/arrow-nanoarrow.git path/to/arrow/nanoarrow"
+  echo "Not building nanoarrow because '${source_dir}' does not exist"


I'm not fond of this, because it implies that a typo when invoking this script, or a missing checkout, will go unnoticed.

pitrou · 2024-06-13T13:03:51Z

ci/scripts/nanoarrow_build.sh

@@ -27,18 +27,21 @@ build_dir=${2}/nanoarrow
 # integration tests. Testing of the nanoarrow implementation in normal CI is handled
 # by github workflows in the arrow-nanoarrow repository.

+# Include in the integration test by default if the source directory is present


The problem is that the flag is still off by default in Archery:

arrow/dev/archery/archery/cli.py

Lines 741 to 743 in 680980e

@click.option('--with-nanoarrow', type=bool, default=False,

help='Include nanoarrow in integration tests',

envvar="ARCHERY_INTEGRATION_WITH_NANOARROW")

vibhatha · 2024-06-15T04:25:43Z

docker-compose.yml

@@ -1748,8 +1748,6 @@ services:
    volumes: *conda-volumes
    environment:
      <<: [*common, *ccache]
-      ARCHERY_INTEGRATION_WITH_NANOARROW: 0


Just curious. Should we mark it =1 here and let the script take the passed value?

That's a great suggestion! I think if that happens, then the script will error if somebody doesn't have a nanoarrow checkout (and so a local run of archery docker run ... will fail perhaps unexpectedly).

paleolimbot · 2024-06-17T15:20:17Z

@vibhatha @pitrou Thank you for taking a look! I think the right thing to do is probably to run the integration tests from the nanoarrow repo where there are fewer arrow-related constraints (e.g., copy what's done in the arrow-rs repo).

The right thing to do here is to apply @kou's suggestion to fix the root cause (unexpected behaviour of archery docker run); however, it looks like this causes problems in other places (or perhaps I applied the suggestion incorrectly!).

kou · 2024-06-18T02:07:31Z

Ah, sorry. My patch was wrong. How about this?

diff --git a/dev/archery/archery/docker/core.py b/dev/archery/archery/docker/core.py
index ec422e9aaf..5be4887ea4 100644
--- a/dev/archery/archery/docker/core.py
+++ b/dev/archery/archery/docker/core.py
@@ -383,10 +383,6 @@ class DockerCompose(Command):
                     args.append(f'--memory={memory}')
                     args.append(f'--memory-swap={memory}')
 
-            # get the actual docker image name instead of the compose service
-            # name which we refer as image in general
-            args.append(service['image'])
-
         if user is not None:
             args.extend(['-u', user])
 
@@ -399,6 +395,10 @@ class DockerCompose(Command):
                 args.extend(['--volume', volume])
 
         if use_docker:
+            # get the actual docker image name instead of the compose service
+            # name which we refer as image in general
+            args.append(service['image'])
+
             # add command from compose if it wasn't overridden
             if command is not None:
                 args.append(command)

Co-authored-by: Sutou Kouhei <[email protected]>

paleolimbot · 2024-07-15T19:46:15Z

@kou This seems to be working...is there any additional changes needed to this approach?

kou

+1

conbench-apache-arrow · 2024-07-16T10:21:32Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 21238a7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 14 possible false positives for unstable benchmarks that are known to sometimes produce them.

paleolimbot requested review from assignUser, kou and raulcd as code owners June 6, 2024 13:04

paleolimbot marked this pull request as draft June 6, 2024 13:05

github-actions bot added the awaiting committer review Awaiting committer review label Jun 6, 2024

paleolimbot marked this pull request as ready for review June 6, 2024 16:21

kou reviewed Jun 6, 2024

View reviewed changes

ci/scripts/nanoarrow_build.sh Outdated Show resolved Hide resolved

ci/scripts/rust_build.sh Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting committer review Awaiting committer review awaiting changes Awaiting changes labels Jun 6, 2024

paleolimbot force-pushed the ci-integration-rust-na branch from fad0312 to 7dbef9d Compare June 13, 2024 12:50

pitrou reviewed Jun 13, 2024

View reviewed changes

vibhatha reviewed Jun 15, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 17, 2024

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 18, 2024

paleolimbot force-pushed the ci-integration-rust-na branch 2 times, most recently from 1b938b8 to 932ccdc Compare June 24, 2024 13:51

paleolimbot added 5 commits July 15, 2024 11:39

try specifying via env and not via command line

f5157cf

one more approach

365a4a6

ensure int expression

e9c0994

maybe fix rust version

530d5ce

maybe use the old one by default

82f6eea

paleolimbot and others added 5 commits July 15, 2024 11:39

try specifying

8b2f972

Apply suggestions from code review

d71259c

Co-authored-by: Sutou Kouhei <[email protected]>

revert changes

e4b1fa1

apply kou's suggestion

1c1b7c1

try kou's suggestion

74b2f96

paleolimbot force-pushed the ci-integration-rust-na branch from 932ccdc to 74b2f96 Compare July 15, 2024 14:39

kou approved these changes Jul 16, 2024

View reviewed changes

kou merged commit 21238a7 into apache:main Jul 16, 2024
53 checks passed

kou removed the awaiting change review Awaiting change review label Jul 16, 2024

github-actions bot added the awaiting merge Awaiting merge label Jul 16, 2024

kou mentioned this pull request Jul 16, 2024

[Archery][CI][Integration] Integration workflow is no longer running Rust integration tests #41481

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-41481: [CI] Update how extra environment variables are specified for the integration test docker job #42009

GH-41481: [CI] Update how extra environment variables are specified for the integration test docker job #42009

paleolimbot commented Jun 6, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Jun 6, 2024

kou left a comment

paleolimbot commented Jun 11, 2024

pitrou Jun 13, 2024

pitrou Jun 13, 2024

vibhatha Jun 15, 2024

paleolimbot Jun 17, 2024

paleolimbot commented Jun 17, 2024

kou commented Jun 18, 2024

paleolimbot commented Jul 15, 2024

kou left a comment

conbench-apache-arrow bot commented Jul 16, 2024

	@click.option('--with-nanoarrow', type=bool, default=False,
	help='Include nanoarrow in integration tests',
	envvar="ARCHERY_INTEGRATION_WITH_NANOARROW")

GH-41481: [CI] Update how extra environment variables are specified for the integration test docker job #42009

GH-41481: [CI] Update how extra environment variables are specified for the integration test docker job #42009

Conversation

paleolimbot commented Jun 6, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Jun 6, 2024

kou left a comment

Choose a reason for hiding this comment

paleolimbot commented Jun 11, 2024

pitrou Jun 13, 2024

Choose a reason for hiding this comment

pitrou Jun 13, 2024

Choose a reason for hiding this comment

vibhatha Jun 15, 2024

Choose a reason for hiding this comment

paleolimbot Jun 17, 2024

Choose a reason for hiding this comment

paleolimbot commented Jun 17, 2024

kou commented Jun 18, 2024

paleolimbot commented Jul 15, 2024

kou left a comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Jul 16, 2024

paleolimbot commented Jun 6, 2024 •

edited by github-actions bot

Loading